Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue two tasks upon finishing ICE gathering, and fire gatheringstatechange & icegatheringstatechange in same task #2894

Merged
merged 11 commits into from
Mar 29, 2024

Conversation

jan-ivar
Copy link
Member

@jan-ivar jan-ivar commented Aug 21, 2023

Fixes #2892.
Fixes #2893.
Fixes #2896.

cc @docfaraday


Preview | Diff

@jan-ivar jan-ivar self-assigned this Aug 21, 2023
Copy link
Contributor

@alvestrand alvestrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.
The general principle of state-setting seems to be that we change the state before we fire the event that informs the user of the state change; the old code violated this, so was clearly wrong.

@docfaraday
Copy link
Contributor

I think we might be missing the steps that add an a=end-of-candidates to the local description.

@docfaraday
Copy link
Contributor

Also, it looks like we'll fire duplicate null candidates here. We should only fire the null candidate if the RTCPeerConnection.iceGatheringState just transitioned to "complete".

@jan-ivar
Copy link
Member Author

jan-ivar commented Aug 23, 2023

I think we might be missing the steps that add an a=end-of-candidates to the local description.

Isn't that covered by JSEP? This spec generally doesn't get into that. In any case that seems like a separate issue.

@docfaraday
Copy link
Contributor

I think we might be missing the steps that add an a=end-of-candidates to the local description.

Isn't that covered by JSEP? This spec generally doesn't get into that. In any case that seems like a separate issue.

Hmm. So JSEP handles this when creating a new local description, but does not update the existing local description. Maybe we shouldn't be updating? Anyhow, yeah, separate issue.

@jan-ivar
Copy link
Member Author

duplicate null candidates

Ah, I forgot to remove the now unused "update the ICE gathering state" algorithm. Thanks for catching that!

@youennf youennf added the Needs Test Needs a WPT test label Aug 24, 2023
@alvestrand
Copy link
Contributor

Seems good to merge. Needs test.

@jan-ivar
Copy link
Member Author

@jan-ivar jan-ivar removed the Needs Test Needs a WPT test label Mar 28, 2024
@jan-ivar
Copy link
Member Author

Hi @dontcallmedom I'm getting this error. It says to see developer console, where can I see that? Trying to figure out how to bother you less ;)

  Error: ROR] Function listAmendments threw an error during preProcess.
        Plugin: core/pre-process
          Hint: See developer console.

Also the next step "Check amendment annotations on substantive changes to Rec / check-amendment (pull_reques" seems to have passed, which seems odd if it failed on amendments.

@dontcallmedom
Copy link
Member

I'm getting this error. It says to see developer console, where can I see that? Trying to figure out how to bother you less ;)

The way I check it is by running the ReSpec document in my local browser (served via a local http server since otherwise you'd hit CORS issues). The error in the developer console says:

The container with id rtcicetransport marked as amended cannot embed the other container of amendment rtcicetransportstate-failed-description, see https://github.com/w3c/webrtc-pc/blob/main/amendments.md for amendments management

The problem here is that you're applying the change to the whole "rtcicetransport" section, which contains already a place with another change; I think the solution would probably to add a <div> wrapping the more specific changes; I think you would also need to markup the deletion of the "Update the ICE gathering state" section.

I'm happy to leave it to you, or do it on my end as you prefer.

Also the next step "Check amendment annotations on substantive changes to Rec / check-amendment (pull_reques" seems to have passed, which seems odd if it failed on amendments.

That job's role is only to check that a non-editorial PR is listed in amendments; let me know if you see a better name to use for the check.

@jan-ivar jan-ivar merged commit cfd8322 into w3c:main Mar 29, 2024
3 checks passed
@jan-ivar jan-ivar deleted the syncgathering branch March 29, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants